FAO @artokin

This PR is partly for testing, and to initiate discussion. I would personally like to see wait_ms() changed, or an alternative call in mbed_wait_api() brought in to avoid the unwanted CPU-sapping spin.

See please and chime in there: #5194 (comment) . We shall create an issue for this?

Looks like you're thinking along the same lines as me, but for different reasons. Yes, I guess this should be an issue.

I agree with your proposal in that comment - wait_ms should be millisecond-precision and thread-sleepy, while wait_us is microsecond-precision and spinny. (Could conceivably be non-spinny using mbed_ticker_api?).

Not sure what to do with wait() - threshold based on value being less than 0.001? I've seen people doing wait(10e-6).

The reason for this change is that a blocking UARTSerial operation wants to poll quite frequently - here every millisecond, but because of the wait_ms(1) implementation, we'll end up spinning half the time on average. And worse, we could settle in to mostly-spinning or mostly-sleeping for prolonged periods, leading to noticeably different behaviour depending on which phase of the clock you first call the block.

cc @bulislaw @c1728p9 @pan-

(And yes, obviously it should be using a proper wake-up mechanism rather than once-a-millisecond polling, but the first draft at such a mechanism got tangled up in review, and the ultimate solution which also handles ::poll requires Russ's condition variables or something like them. This is an interim improvement to the current interim functionality.)

I agree having two different wait behaviors and 4 different wait calls is a bit confusing. Especially that wait() and Thread::wait() are different.

We can't do much about existing API right now, because of compatibility issues. But I've noticed that:

/** Waits a number of milliseconds.
 *
 *  @param ms the whole number of milliseconds to wait
 */
void wait_ms(int ms);

Is implemented incorrectly as

void wait_ms(int ms) {
    wait_us(ms * 1000);
}

The API doc says Waits a number of milliseconds. and it's implemented with possible sub-ms wait. We could just change the implementation to only include ms wait I would say.

Making it just call Thread::wait(ms) is what I would expect and want, but it is an effective API change and difference from the non-RTOS one and current RTOS one.

wait_ms(1) for the current implementations will wait between 0.999ms and 1.000ms. It's guaranteeing both precision, and not undershooting (by more than a microsecond)
Thread::wait_ms(1) would wait between 0.000 and 1.000ms, depending on current tick phase.

Agreed, implementing wait_ms in terms of system tick, right now, would change the behaviour quite a bit and break the compatibility.

you could avoid breaking some of the backwards compatibility by delaying for an extra ms -
Thread::wait(ms + 1). That way wait_ms(1) would be guaranteed to wait at least 1.00ms and worst case 2.00ms. The worst case is no worse than a thread of equal priority stealing a tick due to round-robin scheduling.

As for this PR, the change looks good to me. That way UARTSerial won't be blocked on the wait_ms discussion.

Unfortunately adding one could cause significant problems the other way - people trying to construct a 10ms periodic thing with a 10ms wait would get significant periodic drift (they'd have some anyway, but this would add a lot). Expecting them to specify "9" to get a 10ms period would be counterintuitive.

See this discussion here for people tearing their hair out over a built-in add 1 to RTX's osDelay that was quickly reverted. This scenario isn't quite the same, but I think the discussion is relevant. http://www.keil.com/forum/60393/

Not adding one is probably what the majority of users want - they'd get the period they expect with repeated calls. But there will be people wanting a minimum delay for some HW tolerance requirement...

I'd say "change to ticks + 1" is my least favourite of the 3 options for wait_ms. Haven't got a strong preference on the other two though.

• leave as-is (deprecate?), introduce new documented-as-potentially-tick-based API
• change to wait ticks
• change to wait ticks + 1

I don't think we can do much right now, except maybe implementing the functions in terms of the tickers. Going forward making both wait APIs work in the same way when they have the same name and having separate higher res (spining) wait function would make sense.

Going back to the subject of the PR, there's also poll(), which like the block code in UARTSerial is indeed polling due to the lack of a wake-up mechanism.

And it really is spinning with just a yield - to make it perform the same as the block, I'll change that to a 1 millisecond wait too.

Going back to the subject of the PR

So far @c1728p9 reviewed, and me (LGTM). Anyone else ? We are having jenkins CI issues currently that will be fixed ,same for circle CI. Then we can trigger morph build.

@kjbracey-arm Could you change the sha of the last commit to kick out circle ci?

/morph test-nightly

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1554

All builds and test passed!

Build : FAILURE

Build number : 34
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5216/

/morph build

Build : SUCCESS

Build number : 103
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5216/

Triggering tests

/test mbed-os

Test : SUCCESS

Build number : 49
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/5216/49